Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cargo test with extension-module feature. #1123

Closed
wants to merge 1 commit into from

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Aug 30, 2020

The excellent news is that rust-lang/cargo#8441 will resolve a lot of our existing issues with extension-module. The bad news is that it's not yet merged, so please wait with merging this for now!

I'm not sure if this solves #904 (proc-macro crate) - we'll have to test and perhaps refine. I was able to confirm locally that cargo test and cargo run are fixed with this feature.

Fixes #1084
Fixes #771
Fixes #341
Fixes #340

@programmerjake
Copy link
Contributor

Will this prevent testing pyo3 code on rust stable due to -Zextra-link-arg being unstable?

@davidhewitt
Copy link
Member Author

I don't think so, my experience just now was that cargo ignores the directive printed by the build script if it doesn't recognize it.

@gilescope
Copy link
Contributor

@programmerjake its nice to have worries like that. So nice that pyo3 is available on stable. So much great work done this year!

@davidhewitt
Copy link
Member Author

As the cargo PR just got merged, I've rebased this and once I've confirmed it works on Rust nightly I'll update docs and propose to merge this.

@Spirans
Copy link

Spirans commented Mar 4, 2021

Hello!
I really appreciate this project and worrying a little about this PR, because my team has exactly the same issue.
@davidhewitt so is everything fine with night build? Do you need some help with something about that PR?

@davidhewitt davidhewitt force-pushed the rustc-link-arg-bins branch 2 times, most recently from bc794b2 to b0cf4cc Compare March 4, 2021 11:34
@davidhewitt
Copy link
Member Author

Hi @Spirans thanks for the check-in.

I rebased this PR again just now just to confirm what I last found. Basically we need some of the build-script variables from rust-lang/cargo#8441 (comment) for this PR: specifically at the moment this uses rustc-link-arg-bins, which fixes cargo run, but we also need rustc-link-arg-tests to fix cargo test.

According to rust-lang/cargo#8441 (comment) the PR which implemented rustc-link-arg-bins added "the plumbing" to support rustc-link-arg-tests, but didn't implement it.

I was planning to write an upstream cargo PR to implement rustc-link-arg-tests (I guess it should be quite easy as it should just be copying the rustc-link-arg-bins code and modifying a little). However I'm quite busy so didn't get around to writing that PR yet.

Do you need some help with something about that PR?

If you've got the time and want to push this PR through faster, would you be willing to write the cargo PR?

@Spirans
Copy link

Spirans commented Mar 4, 2021

@davidhewitt thank you!
I'll take a look at that fix and will probably implement rustc-link-arg-tests soon.

Base automatically changed from master to main March 16, 2021 22:09
@davidhewitt davidhewitt force-pushed the rustc-link-arg-bins branch 2 times, most recently from 0943151 to 089ad42 Compare April 6, 2021 18:34
@davidhewitt
Copy link
Member Author

FYI @Spirans I just had a chance this morning to write the cargo PR: rust-lang/cargo#9416

@svenstaro
Copy link
Contributor

Any chance to get this cleaned up and merged?

@davidhewitt
Copy link
Member Author

I will complete the cargo PR first (hopefully soon) and then once the support is there in Rust nightly I will finish this off.

@davidhewitt
Copy link
Member Author

Some updates to this:

  • The cargo PR I opened got closed due to inactivity 🤦 . I'll pick it up again soon and push it through, now that 0.14.2 fixes are out of the way.
  • The learning from Tracking issue for rustc-cdylib-link-arg transitive warning rust-lang/cargo#9562 is that what this PR adds to PyO3's build scripts is not actually correct. Instead, these link args must be added to the final crate. Most likely, pyo3_build_config::add_extension_module_link_args() can have these added.

@aviramha
Copy link
Member

aviramha commented Jan 7, 2022

The linked PR seems to be merged? Can we get this done? This will be great!

@davidhewitt
Copy link
Member Author

Next step here is for someone to reboot my closed PR rust-lang/cargo#9416, taking on the feedback I received and see it across the finish line.

I've been meaning to do it for absolutely ages, and will do it eventually, but if anyone wants to volunteer to take that on then that'll definitely help this come sooner!

@aviramha
Copy link
Member

aviramha commented Jan 8, 2022

Next step here is for someone to reboot my closed PR rust-lang/cargo#9416, taking on the feedback I received and see it across the finish line.

I've been meaning to do it for absolutely ages, and will do it eventually, but if anyone wants to volunteer to take that on then that'll definitely help this come sooner!

I'll pick it. Great way to contribute for the first time to Rust repositories !

@aviramha aviramha self-assigned this Jan 8, 2022
@aviramha
Copy link
Member

aviramha commented Jan 8, 2022

@davidhewitt
Copy link
Member Author

Brilliant, thank you!

@aviramha
Copy link
Member

@davidhewitt Please check my last comment in rust-lang/cargo#10274 . I want to make sure I explained the use case correctly ;)

@aviramha
Copy link
Member

It's merged now! I assume we need to wait for it to get to stable?

@davidhewitt
Copy link
Member Author

Fantastic, thanks so much for getting this moving!

I'd be ok with finding a way for this feature to use nightly for now (it's easy enough to ask users who want this to use cargo +nightly test). We just need to wait for rust nightly to update the pinned cargo (which happens every few weeks I think).

Note that due to the way the extra-link-arg options now work, the approach in this PR isn't quite correct. Instead, the build script of the final crate needs to be the one which has these linker arguments.

I actually wrote pyo3_build_config::add_extension_module_link_args with exactly this in mind - so I suggest the next steps after this is available in nightly would be to add to that function to emit the additional link args added by this PR. @aviramha would you be interested in taking that on? I can explain in more detail as you need.

@aviramha
Copy link
Member

Sure I can take it.

@aviramha
Copy link
Member

Closing this draft, see new PR #2135

@aviramha aviramha closed this Jan 31, 2022
@davidhewitt davidhewitt deleted the rustc-link-arg-bins branch February 5, 2022 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment